Conversation
- Treat missing/NaN MPHeight as unknown and set to `None` - Persist a NULL MeasuringPointHistory row whenever MPHeight is missing/NaN, even if an estimate could be derived. - If MPHeight is present, existing estimator-based behavior remains.
- Updated logic to ignore `None` values for `measuring_point_height` and `measuring_point_description`. - Added test to verify correct handling of null measuring point history.
ksmuczynski
changed the title
ksmuczynski
changed the title
There was a problem hiding this comment.
Pull request overview
This PR addresses handling of NULL/NaN values in measuring point data during legacy data transfers. When MPHeight is NaN or missing in the source data, the system now skips estimation and inserts NULL values directly, ensuring complete well migration without unreliable estimator calls. The read path is hardened to skip NULL history entries and return the most recent non-NULL value.
Changes:
- Modified transfer logic to detect NaN values and skip the measuring point estimator for those cases
- Updated
measuring_point_heightandmeasuring_point_descriptionproperties to iterate through history and return first non-NULL value - Added test coverage for NULL history record handling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| transfers/well_transfer.py | Updated _build_well_payload to treat NaN MPHeight as None; refactored _add_histories to conditionally call estimator only for valid MPHeight values and persist NULL records otherwise |
| db/thing.py | Modified measuring_point_height and measuring_point_description properties to skip NULL values in history and return most recent non-NULL value |
| tests/test_thing.py | Added test to verify properties correctly skip NULL history entries and return older non-NULL values |
| if not added_measuring_point: | ||
| raw_desc = getattr(row, "MeasuringPoint", None) | ||
| mp_desc = None if isna(raw_desc) else raw_desc | ||
| session.add( | ||
| MeasuringPointHistory( | ||
| thing_id=well.id, | ||
| measuring_point_height=None, | ||
| measuring_point_description=mp_desc, | ||
| start_date=datetime.now(tz=UTC).date(), | ||
| end_date=None, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This code block that handles the fallback case when the estimator returns no results is duplicated from lines 735-745. Consider refactoring to eliminate this duplication. You could extract the logic for creating a NULL measuring point history record into a helper method or move this fallback outside the if-else block since both branches need it.
| pass | ||
| if mpheight is None or isna(mpheight): | ||
| # Treat missing/NaN MPHeight as unknown during migration. | ||
| mpheight = None |
There was a problem hiding this comment.
When MPHeight is None or NaN, the measuring point description should also be checked for NaN and converted to None before passing to CreateWell. If row.MeasuringPoint contains a pandas NaN value (float), it could fail Pydantic validation since measuring_point_description expects str | None, not a float. Consider adding: mpheight_description = None if isna(row.MeasuringPoint) else row.MeasuringPoint inside the if block for consistency with the handling in _add_histories (lines 735-736).
| mpheight = None | |
| mpheight = None | |
| mpheight_description = ( | |
| None if isna(row.MeasuringPoint) else row.MeasuringPoint | |
| ) |
Why
This PR addresses the following problem / context:
measuring_point_heightand NULL history rows formeasuring_point_descriptioncaused issues during data transfers and property calculations.How
Implementation summary - the following was changed / added / removed:
Nonevalues during property calculations.Notes
Any special considerations, workarounds, or follow-up work to note?
MPHeightNaNs as NULLs (skip estimator) to ensure full well migration #541